[EuiFocusTrap] Use react-focus-on
#3631
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
This looks great! It's already doing what it's supposed to! (Sorry about the janky photos, I can never get it to take a screen shot...) With the old system, here's what a screen reader could still get to: With the new system, screen readers are correctly constrained to just the modal:
|
The lack of documentation is a bummer, but through looking at the code I've seen that For the most part, leaving
|
|
@cchaos EuiCollapsibleNav has a unique approach to trapping and releasing focus, and I want to make sure I understand the intended interaction before making any changes. What I see:
The main question is: is it intentional that clicking the header bar does not close the nav? |
|
Yes, at least for now until we hear back from users, the intention is to keep the nav open when they're interacting with the header. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
jenkins test this
|
This likely won't make this PR. We have no guarantee that a given portal is the last thing in the DOM, so there would be some case-by-case basis decisions to make. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
In an ideal world, flyouts are just modals with a different design. Meaning: But, flyouts default to So, at this point, I think it makes sense for |
|
Confirmed that disabling scroll locking is easy (
|
|
Thoughts only As EuiFocusTrap is an existing component, I have a reservation about making scroll locking and to a lesser extent isolation being on by default. I confirmed with a long focus-trapping form that tabbing between elements always scrolls between them, but it isn't possible to navigate to them otherwise. We don't know and can't test everywhere these trappings are being used in Kibana, let alone other projects. For components we manage like flyout & modal, I think there's a strong argument for enabling those values. Otherwise, I worry a bit about push back and complaints - "everyone was happy with this until EUI changed" - even if, ultimately, this is the right direction. |
|
Good points, @chandlerprall As a utility component that does not prescribe DOM context, perhaps we should default to |
|
Thought some more on the defaults, and I think my primary resistance to isolation & scroll lock is they seem unexpected to the user outside of modal/flyout experiences. If we could add a visual (and likely? screen reader) indication about the active focus lock then I'd be much more okay with that experience, e.g. something like an input element's focus ring. This doesn't necessarily need to be built into EUI's component, instead it could be a provided design pattern. Either way I'd like to get @cchaos's input on this as a whole, especially how these changes may interact with supported edge cases in EUI's component interactions. I would not have a problem merging this & releasing it after Kibana's FF to help give a full minor for finding consequences. |
Definitely. There is no need to rush getting this in. We can come back to this next week and perhaps have a more synchronous conversation now that the scope of implications is more know. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
1 similar comment
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
|
Updated per sync discussion earlier:
We can merge as-is and will get all the |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3631/ |
* Break up release.js (elastic#3723) * Switch release.js to use arguments instead of env vars * Switch MFA code back to env var so it doesn't leak in CI logs * Update job definition to use --type arg * Support breaking up release steps with args * Break release up to fetch time-sensitive MFA token right before publish * Strip whitespace from each step Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * Allow prop-setting on the FlexItems within DescribedFormGroup (elastic#3717) * Allow prop-setting on the FlexItems within DescribedFormGroup * Add changelog entry * Honor classes on fieldFlexItem * Update src/components/form/described_form_group/described_form_group.test.tsx Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update snap name Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Re-focus EuiSuperSelect input after making a value change (elastic#3734) * Re-focus EuiSuperSelect input after making a value change * changelog * [EuiStat] Allow customizing the render of the title and description HTML tags (elastic#3693) * Add titleElement and descriptionElement to EuiStat * Updated snapshots * Updated changelog * Fixed issue with screenreader * Updated snapshots * Use screen reader only if title and description is a string * updated snapshots * Merge remote-tracking branch 'upstream/master' into fix/stat * Fixed typo in changelod * removed titleChildren * titlechildren as variable * Update CHANGELOG.md Remove an extra line left over from a merge resolution Co-authored-by: Chandler Prall <chandler.prall@gmail.com> * [EuiTable] Expand item action name to allow a function (elastic#3739) * allow for item -> ReactNode in name * docs * CL * Add ssh keys so new tags can be pushed to Github from Jenkins (elastic#3740) * Add ssh keys so new tags can be pushed to Github * Need a vault token before we can pull secrets * update i18ntokens * 27.1.0 * Updated documentation. * Chore/decouple button content (elastic#3730) * [New] EuiButtonContent For rendering the contents of buttons, icon (loading spinner) and text wrapper for children * Use EuiButtonContent in EuiButtonEmpty * Fixing classNames and disabled states * Create EuiButtonDisplay for internal usage * Fix snaps * ts gymnastics * Added tests for EuiButtonContent * More optimization - Extend EuiButtonContentProps - Content styles are in button_content.scss * Restricted list of `element`s * [Docs] Adding more acccessibility-focused notes and examples (elastic#3714) * making more a11y callouts in docs * Apply suggestions from code review Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * prettier update Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> * [EuiFocusTrap] Use `react-focus-on` (elastic#3631) * WIP: react-focus-on * WIP: fix GuideFullScreen * noIsolation; onClickOutside * euiflyout snapshot updates * euiflyout snapshot updates * euimodal snapshot updates * euidatagrid snapshot updates * euicolorpalettepicker snapshot updates * euisuperselect snapshot updates * euicollapsible snapshot updates * euifocustrap snapshot updates * allow for array snapshots with takeMountedSnapshot * docs improvements * default to noIsolation=true and scrollLock=false * CL * Fixed bug in EuiComboBox always showing a scrollbar (elastic#3744) * Fixed EuiComboBox always showing a scrollbar * Adding the right PR number to CL * Added useEuiI18n hook (elastic#3749) * Added useEuiI18n hook * Updated docs with useEuiI18n hook, added snippets * Add support to fetch-1i8n-strings or useEuiI18n to match EuiI18n extraction * Fix up return types for useEuiI18n * Updated custom eslint i18n rule/package to lint useEuiI18n usages * changelog * Remove something I was testing with and lost where I had placeed it. * [EuiSuperDatePicker] Bypass formatting `null` dates (elastic#3750) * prevent formatting on null value * remove unnecessary cast * account for keyboard nav with null selection * CL * Updated EuiComboBox to allow the options list to open for single selection custom options (elastic#3706) * Fixing includes to return true when object exists in array * changelog * Allowing list to open for single selection custom options * Updated changelog * PR review * Improving example * Improving example * Addind isClearable * Improving examples * Improving explanation text * Adding note * Addressing PR issues * Update src-docs/src/views/combo_box/combo_box_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Update src-docs/src/views/combo_box/combo_box_example.js Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Snippet Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> * Add analyze event glyph to EuiIcon (elastic#3729) * adding analyze event security icon * updating analyze_event icon * updating again * Update CHANGELOG.md * Update consuming.md (elastic#3769) * Path has changed and the wiki has not been updated. * Fix zIndex for two popup ups (elastic#3768) Clicking both buttons on https://eui.elastic.co/#/tabular-content/data-grid-styling-and-toolbar demo brings up partially hidden popups because their z-index is too low. Increasing by one seems to do the trick. * [Playground] EuiBadge, EuiNotificationBadge, EuiBetaBadge (elastic#3722) * created playground for badges * removed commented code * used validator for iconType and colour * updated variable name * removed colour validation * removed unnecessary imports * removed default values, fullscreen mode * suggesstions * removed placeholder, added required, some badge props set to string * used actual value of child in text field * added sanitize function * fixed unique-key warning * added validation * updated to identify the change whenthe state changes * suggestions * added onCLick function snippet * removed error popup by react-view * removed lint err * removed commented code Co-authored-by: Josh Mock <joshua.mock@elastic.co> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Scott Gould <sbg@elastic.co> Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com> Co-authored-by: Ashik Meerankutty <ashik9591@gmail.com> Co-authored-by: Greg Thompson <thompson.glowe@gmail.com> Co-authored-by: Michail Yasonik <michail.yasonik@elastic.co> Co-authored-by: gchaps <33642766+gchaps@users.noreply.github.com> Co-authored-by: Elizabet Oliveira <elizabet.oliveira@gmail.com> Co-authored-by: Marra Sherrier <45169477+marrasherrier@users.noreply.github.com> Co-authored-by: Alberto Andújar <josealbertoandujar@gmail.com> Co-authored-by: Yuri Astrakhan <yuriastrakhan@gmail.com>
* WIP: react-focus-on * WIP: fix GuideFullScreen * noIsolation; onClickOutside * euiflyout snapshot updates * euiflyout snapshot updates * euimodal snapshot updates * euidatagrid snapshot updates * euicolorpalettepicker snapshot updates * euisuperselect snapshot updates * euicollapsible snapshot updates * euifocustrap snapshot updates * allow for array snapshots with takeMountedSnapshot * docs improvements * default to noIsolation=true and scrollLock=false * CL
This reverts commit cdfa280.
This reverts commit cdfa280.
|
|
Summary
Replace
react-focus-lockwithreact-focus-on, an updated and preferred-by-the-shared-author take on the original.The main improvement EUI will see is enhanced screen reader and keyboard navigation behavior for portal- and popover-based components and contexts.
For the most part, this is a 1:1 replacement, with a couple adjustments to maintain EUI's API.
Additional features include:
noIsolation: for opting into heavy-handedpointer-eventscontrol (non-standard use cases)scrollLock: for opting intobodyscrolling preventiononOutsideClick: callback that gives EUI additional options for event handlingSee #502 for the initial idea and discussion.
Lots of manual testing appreciated
Draft checklist
noIsolationonClickOutsideinstead ofEuiOutsideClickDetectorChecklist
- [ ] Check against all themes for compatibility in both light and dark modes- [ ] Checked in mobile- [ ] Checked Code Sandbox works for the any docs examplesExcluded